Still don't love that it's opt-out instead of opt-in, but I understand why (warnings harm no one, and fixes help everyone). I agree it makes more sense to move this into the Python package.

What I don't like about this conceptually is that the python package imports the cmake module.

Once we do content hashing properly and follow imports, it would mean that Python's hash is affected by changes to the cmake module. That feels wrong to me. I prefer the status quo of having this logic in cmake.py.

Secondly, you've put this under python, but in reality pkg.spec["python"].command picks up python-venv's package. I think the logic is better kept in cmake.py where it chooses python's or python-venv's interpreter path. Putting it in the python package seems to suggest this is about python's interpreter, but it's not.

Secondly, you've put this under python, but in reality pkg.spec["python"].command picks up python-venv's package. I think the logic is better kept in cmake.py where it chooses python's or python-venv's interpreter path. Putting it in the python package seems to suggest this is about python's interpreter, but it's not.

So what is the correct behavior here? I would think that pkg.spec["python"].command is what we want for configuring the dependent.

@haampie are you okay if we just put a comment next to the python setup that says something along the lines of

# this can redirect to python-venv if pkg extends python

I would think that pkg.spec["python"].command is what we want for configuring the dependent.

Sure, but it's very confusing to use pkg.spec["python"].command in a method of class Python. Everyone reading that would expect self.command (used 22 times in the same class).

That tells me that python is not a good use case for this feature right now, so I would undo that change.

Maybe you could write it as

class Python:
    def dependent_cmake_args(self, dependent_spec: Spec) -> List[str]:
        if dependent_spec.dependencies("python-venv"):
            return []
        return [... self.command ...]

...

class PythonVenv:
    def dependent_cmake_args(self, dependent_spec: Spec) -> List[str]:
        return [... self.command ...]

but that's also not satisfactory cause packages would have to opt out of two packages that provide hints instead of one.

That tells me that python is not a good use case for this feature right now, so I would undo that change.

I disagree on a couple points here.

Personally I don't think it is that confusing. It is reasonably well documented at this point that python has some special handling already and the comment links to the PR that has additional details about why this is the case. I spent a couple of seconds reading the PR and understood what was done well enough to understand why it was written that way.

We could also write it is self.spec.command which is what we do for MPI providers for similar cases.

I really don't like package specific code inside of the CMakePackage. The people maintaining that code are not the same people maintaining the oddities of python or other packages and how they are modeled in Spack. This moves the responsibility and context of python changes into python and out of the build system.

@spackbot run pipeline

I've started that pipeline for you!

CI Failures are unrelated to this change. This change is blocking progress on other work @alalazo @haampie are we okay to force merge this to unblock things?

Yeah, I'll go on force merging this - there are just a handful of known flaky jobs that went ❌